Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make session cookie use ASGI root path #1147

Merged

Conversation

mahmoudhossam
Copy link
Contributor

Fixes #233

@JayH5
Copy link
Member

JayH5 commented Mar 11, 2021

Thanks, this seems like a reasonable improvement. It does need a test, I think, though.

Co-authored-by: Tom Christie <tom@tomchristie.com>
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the final remaining suggestion is addressed I'm probably okay with pulling this in.

It could potentially use a test case, but it's probably also okay enough without.

@mahmoudhossam
Copy link
Contributor Author

I'm going to add a test and then we can merge this, thanks for the quick response!

@mahmoudhossam
Copy link
Contributor Author

I couldn't find a way to set the root path directly on the app so I had to mount another app on the main one and tested that, seems to work as expected.

@tomchristie
Copy link
Member

Ah fantastic, yup.

@tomchristie tomchristie merged commit 5ee04ef into encode:master Mar 12, 2021
@mahmoudhossam mahmoudhossam deleted the cleanup/session-cookie-asgi-root-path branch March 12, 2021 13:04
@devsetgo
Copy link

devsetgo commented Jul 23, 2021

@mahmoudhossam or @tomchristie This update broke my session checking function to ensure users are logged in. I mount all the sub routes (users, etc..). The request.session has data in it when set, but when I redirect back to the start page then session is now null. I see no documentation on how to have the session across Mounts?

Below is example of what I am doing...

# Routes
routes = [
    Route("/", main_pages.homepage, name="dashboard", methods=["GET", "POST"]),
    Route("/about", main_pages.about_page, methods=["GET"]),
    Mount("/user", routes=
    [
        Route(
            "/forgot", routes=user_pages.forgot_password, methods=["GET", "POST"]
        ),
        Route("/login", routes=user_pages.login, methods=["GET", "POST"]),
        Route("/logout", routes=user_pages.logout, methods=["GET", "POST"]),
        Route(
            "/password-change",
            routes=user_pages.password_change,
            methods=["GET", "POST"],
        ),
        Route("/profile", routes=user_pages.profile, methods=["GET"]),
        Route("/register", routes=user_pages.register, methods=["GET", "POST"]),
    ],
    name="user",
    ),

# Session Checking
def require_login(endpoint: Callable) -> Callable:
    async def check_login(request: Request) -> Response:
        if "user_name" not in request.session:
            logger.error(
                f"user page access without being logged in from {request.client.host}"
            )
            return RedirectResponse(url="/user/login", status_code=303)

        else:
            one_twenty = datetime.now() - timedelta(
                minutes=config_settings.login_timeout
            )
            current: bool = one_twenty < datetime.strptime(
                request.session["updated"], "%Y-%m-%d %H:%M:%S.%f"
            )

            if current == False:
                logger.error(
                    f"user {request.session['user_name']} outside window: {current}"
                )
                return RedirectResponse(url="/user/login", status_code=303)

            # update datetime of last use
            logger.info(
                f"user {request.session['id']} within timeout window: {current}"
            )
            request.session["updated"] = str(datetime.now())
        return await endpoint(request)

    return check_login

@mahmoudhossam
Copy link
Contributor Author

I think there should be a difference between mounting routes and mounting whole applications.

I'd expect routes to share everything with the parent application, but submounted applications should have their own session.

@tomchristie thoughts?

@devsetgo
Copy link

I think there should be a difference between mounting routes and mounting whole applications.

I'd expect routes to share everything with the parent application, but submounted applications should have their own session.

@tomchristie thoughts?

I agree, the documentation shows using Mount with a sub app (Mount("/static", app=StaticFiles(directory="static"), name="static"), but without defining "app=x" it is just Submounting routes. For the session, it should check if submounted as an app or route.

@devsetgo
Copy link

devsetgo commented Aug 5, 2021

@tomchristie ?

@mahmoudhossam
Copy link
Contributor Author

@devsetgo I'd say open a new issue describing your use case and make sure to mention that this PR broke your flow so it'd be easy to track for the maintainers.

This PR is already closed and I don't think anyone will be checking it anymore.

@tomchristie
Copy link
Member

Okay folks - got a resolution to this here... #1512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session cookie should use root path
4 participants